-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Store additional properties in ASE calculator #658
base: master
Are you sure you want to change the base?
Conversation
Why did you go with this approach instead of automatically storing energy/energies when computing the forces and stress? The fact that one has to explicitly set these is not ideal, and I feel like a solution to #655 that works without explicit user action would be nicer. |
1797ad8
to
42a8abd
Compare
This was not intended as a fix for #655 (although it is if one knows the internals). It's just to propagate potentially more outputs to the user |
Ok, the initially intended API for doing outputs outside of ASE specification was the |
It looks to me like it recomputes everything... |
No, because this function is not integrated with the rest of ASE interface ( calculator = MetatensorCalculator(...)
outputs = {...}
calculator.run_model(atoms, outputs) Since the user is explicitly calling this function themself, it will only recompute if the users call it again manually. In general there are two separate issues IMO: #655 is about storing multiple that ASE knows about (storing energy when computing forces, etc.); while We need a fix for #655, but I don't think that |
The fact that it is not integrated with the rest of ASE is why the recomputations are necessary. If I need a quantity at some step of MD, I have to recompute, while in principle the same quantity would have been available together with the energy computation at near-zero overhead |
stress_values = -strain.grad.reshape(3, 3) / atoms.cell.volume | ||
stress_values = stress_values.to(device="cpu").to(dtype=torch.float64) | ||
self.results["stress"] = _full_3x3_to_voigt_6_stress( | ||
stress_values.numpy() | ||
) | ||
|
||
def request_properties_every_n_steps( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think this should be part of the same object. The calculator should follow the ASE API for calculators, and we should provide a separate tool (class, function, …) to handle calculations that have to run at other times.
@@ -73,6 +74,7 @@ def __init__( | |||
extensions_directory=None, | |||
check_consistency=False, | |||
device=None, | |||
properties_to_store: Optional[List[str]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
properties_to_store: Optional[List[str]] = None, | |
extra_metatensor_outputs: Optional[Dict[str, ModelOutput]] = None, |
I would rename this and handle it a bit differently from "normal" properties to make it clearer that this mechanism is only intended for things which are not already known ASE properties.
IMO the data should also be stored as TensorMap in calc.results["extra_metatensor_outputs"]["..."]
, again to make it clear we are operating outside of the ASE interface.
These changes allow for a user to calculate and store non-standard properties in the ASE calculator. For relatively experienced users, this is also a fix for #655 (one can set
"energy", "forces", "stress"
asproperties_to_store
, these will be calculated at each step, and then the inheritedget_property
function will not re-trigger a calculation but just read from thecalc.results
dict).Contributor (creator of pull-request) checklist
Reviewer checklist
📚 Download documentation preview for this pull-request